Skip to content

Stop concurrent withdrawals from grabbing the same UTXO - #569

Open
RodriFS wants to merge 2 commits into
mainfrom
fix-concurrent-utxo-selection
Open

Stop concurrent withdrawals from grabbing the same UTXO#569
RodriFS wants to merge 2 commits into
mainfrom
fix-concurrent-utxo-selection

Conversation

@RodriFS

@RodriFS RodriFS commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

What was going wrong

Two withdrawals from the same wallet, submitted at nearly the same time, could pick the same UTXO. When that happened one of the two transactions replaced the other on the network (RBF) instead of both going through.

The cause was a gap between two steps: NodeGuard would read which UTXOs were free, choose some, and only afterwards mark them as taken. Two requests that both did the reading before either did the marking would happily choose the same coin. This affected both automatic coin selection and the "use exactly these UTXOs" flow (Changeless withdrawals and channel opens, e.g. from the UI's UTXO selector) — the latter had no protection at all.

What changed

Choosing UTXOs and claiming them is now a single operation that can't be interleaved with another one. There are two entry points, depending on who picks the coins:

  • LockUTXOs — the caller already knows which UTXOs it wants. Checks nothing else has them, then claims them.
  • SelectAndLockUTXOsAsync — the service reads what's available, picks enough to fund the request, and claims it.

Both hold a mutex for their whole duration, so the read/choose/claim sequence is indivisible. The mutex is released before callers go on to build a PSBT, since that's the slow part and doesn't need protecting.

A few things fall out of that:

  • Fee bumping (RBF) deliberately reuses the UTXOs of the request it replaces, so that specific case is still allowed through. Only a UTXO held by some other request is rejected.
  • Picking a UTXO someone else already took now surfaces as a message ("already locked or frozen") rather than silently going ahead — over gRPC as FailedPrecondition, in the UI as an error toast.
  • Waiting on the mutex has a timeout. If a future change ever introduces a deadlock, it fails with a clear error instead of hanging forever.

Also fixed a pre-existing bug found along the way: the query that ignores one specific request when checking for held UTXOs had a parenthesisation mistake, so the "ignore" only actually worked when that request happened to be in one particular state. Nothing exercised that combination before; the fee-bump case above is the first thing that relies on it.

Notes for review

  • The mutex is process-wide rather than per-wallet. Selections for different wallets now wait on each other, which costs a little parallelism, but a selection is only a handful of short database/NBXplorer reads and these are human-initiated operations. The per-wallet version needed a keyed lock table with reference counting and cleanup, and it wasn't worth the complexity.
  • UTXOs are claimed slightly earlier than before — before the PSBT is built rather than after. That has to happen inside the same protected step as choosing them. If the build then fails, the request is marked as failed, and failed requests don't hold UTXOs, so they free themselves.
  • WithdrawAllFunds still works out its amount outside the protected step, as it did before this PR. If another request takes some coins in between, the amount goes stale and the selection can no longer cover it — that's detected and fails the request with an error rather than under-funding it, so the exposure is a rare retry, not a wrong transaction.

Testing

Unit tests cover the locking and conflict rules, plus the pre-existing bug above (across every state it applies to).

Two end-to-end tests run against a live NodeGuard and bitcoind:

  • many concurrent withdrawals from one wallet, asserting no two transactions ever share an input
  • two withdrawals that explicitly name the same UTXO, asserting exactly one wins and the other is cleanly rejected

dotnet test test/NodeGuard.Tests --filter "Category!=E2E" for the unit tests; just test-e2e for the rest.

@RodriFS
RodriFS marked this pull request as draft August 17, 2026 15:04
@RodriFS
RodriFS force-pushed the fix-concurrent-utxo-selection branch from 844527d to b08467d Compare August 17, 2026 15:50
@RodriFS
RodriFS requested review from Jossec101 and markettes August 17, 2026 15:51
@RodriFS
RodriFS force-pushed the fix-concurrent-utxo-selection branch from b08467d to 9837702 Compare August 17, 2026 15:51
@RodriFS
RodriFS marked this pull request as ready for review August 17, 2026 15:51
@RodriFS
RodriFS marked this pull request as draft August 17, 2026 16:00
@RodriFS
RodriFS force-pushed the fix-concurrent-utxo-selection branch from 9837702 to 8c57b14 Compare August 17, 2026 16:00
@RodriFS
RodriFS marked this pull request as ready for review August 17, 2026 16:01
@RodriFS
RodriFS marked this pull request as draft August 17, 2026 16:34
@RodriFS RodriFS changed the title Serialize UTXO selection per wallet to prevent concurrent double-spend/RBF Prevent concurrent double-locking of UTXOs across automatic and manual selection Aug 18, 2026
@RodriFS
RodriFS force-pushed the fix-concurrent-utxo-selection branch from 14c43c2 to c603282 Compare August 18, 2026 15:29
@RodriFS RodriFS changed the title Prevent concurrent double-locking of UTXOs across automatic and manual selection Stop concurrent withdrawals from grabbing the same UTXO Aug 18, 2026
@RodriFS
RodriFS force-pushed the fix-concurrent-utxo-selection branch from aed9f50 to 8fdfd1c Compare August 18, 2026 16:10
@RodriFS
RodriFS marked this pull request as ready for review August 18, 2026 16:13
Comment thread test/NodeGuard.Tests/E2E/ConcurrentWithdrawalE2ETests.cs

@manumonti manumonti left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🎸

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants